FIX Validate Category / Tag Titles #700
Conversation
Co-authored-by: Michal Kleiner <[email protected]>
There was a problem hiding this comment.
@silverstripe/core-team
What's our thoughts about this in regards to semver?
Specifically, changing the validation rules for whether an empty value is valid or not.
On one hand, it seems like there's no valid reason to have an empty category/tag - but on the other, someone could very well be going BlogTag::create()->write() in a unit test, if the value of the tag doesn't matter for that test.
| @@ -72,6 +72,10 @@ public function validate() | |||
| $validation->addError($this->getDuplicateError(), self::DUPLICATE_EXCEPTION); | |||
| } | |||
|
|
|||
| if (empty($this->Title)) { | |||
| $validation->addError($this->getEmptyTitleError(), self::EMPTY_TITLE_EXCEPTION); | |||
There was a problem hiding this comment.
| $validation->addError($this->getEmptyTitleError(), self::EMPTY_TITLE_EXCEPTION); | |
| $validation->addError($this->getEmptyTitleError(), self::EMPTY_TITLE_EXCEPTION ?? 'EMPTY_TITLE'); |
The constant won't be declared on any custom classes that use this trait, and requiring it to be added is technically a breaking change.
Note: Please test that this actually works - I've not tried the null coalescing operator in this context before. I think it'll work but it'll pay to check.
There was a problem hiding this comment.
Yikes, shouldn't be calling self::<const> on a trait. Not a great choice when this was made back in the day
I don't think null coalescing works here
class C { function F(){ var_dump(self::Z ?? 'default'); } }; (new C)->F();
// PHP Warning: Uncaught Error: Undefined constant C::Z in php shell cod
No one other than core team is going to write new unit tests, and the existing unit tests are green on this PR, so I see no reason to be concerned here. |
People write unit tests for their own projects and modules all the time. It's not super likely those tests will rely on blog tags or categories but it's also not impossible. |
|
Oh sorry I meant "No one other than core team is going to write new unit tests for the blog module" :-) What's the concern? That a project will create their own unit test that instantiates a their own subclass of a While we're very strict with respecting semver API in minor releases, historically we've been somewhat lenient/pragmatic with behavioral changes, including changes to default configuration values. This PR seems at the low end of changes within a minor. |
Not bothered by this particular change, I agree with Steve on this. |
|
Sweet, just wanted to get the general feeling about it, since I was on the fence about it. |
|
@freezernick Are you still interested in this pull request being merged? If so, please make changes to how the constant is applied as requested above. |
|
I'm closing this due to inactivity. If you want to work on it again I'll be happy to reopen it, or you can open a new pull request from scratch. |
Closes #695